Skip to content

Comments

Parsing Error messages refactor#331

Open
johnny-keker wants to merge 8 commits intomasterfrom
err-list-mult-loc
Open

Parsing Error messages refactor#331
johnny-keker wants to merge 8 commits intomasterfrom
err-list-mult-loc

Conversation

@johnny-keker
Copy link
Contributor

@johnny-keker johnny-keker commented Sep 12, 2022

This PR provides some code refactor for Error messages parsing logic.

Notable change: we do not handle error formats in which location are mentioned after error text.

@timlathy
Copy link
Contributor

Good news: with the latest changes to our tooling, messages like *W,code: text (C:\Absolute\Path\source.c:3, C:\Absolute\Path\source.c:5, C:\Absolute\Path\include.h:10) will no longer be generated, so there's no need to handle this format.

I would suggest keeping most of the changes in this PR with the exception of the code that handles locations after error text (it can be safely removed). I would also prefer to go back to the previous signature of Parse... methods (have them return Message or null). I think it's a good style to keep functions pure by avoiding in-out parameters where it's possible.

@johnny-keker
Copy link
Contributor Author

Good news: with the latest changes to our tooling, messages like *W,code: text (C:\Absolute\Path\source.c:3, C:\Absolute\Path\source.c:5, C:\Absolute\Path\include.h:10) will no longer be generated, so there's no need to handle this format.

I would suggest keeping most of the changes in this PR with the exception of the code that handles locations after error text (it can be safely removed). I would also prefer to go back to the previous signature of Parse... methods (have them return Message or null). I think it's a good style to keep functions pure by avoiding in-out parameters where it's possible.

Thanks! Is 9fd8cb2 works for you?

@johnny-keker johnny-keker changed the title Error List multiple locations errors fix Parsing Error messages refactor Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants